Skip to content

fix(emit): Include class names that compose other classes #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 7, 2019

Conversation

bhollis
Copy link
Contributor

@bhollis bhollis commented Apr 30, 2019

Release 2.0.1 breaks emitting definitions for classes that compose classes from another file. This is because the updated regex is looking for /, but css-loader will produce code that looks like this for composed classes:

"composedClass": "_2zoTMkioeCjgqsVq6W5ljQ " + require("-!../../node_modules/css-loader/index.js??ref--4-1!./index.css").locals["baseClass"] + ""

This adds a test for composed classes and fixes the bug. I had to upgrade Jest as well because the existing version could not run on my version of Node (11.11.0).

export = cssExports;
"
`);
expect(declaration).toMatchSnapshot();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the fix has broken this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just switched that one to a snapshot instead of an inline snapshot. I’m not sure why it was inline before vs. the external snapshot for the other case.

@bhollis
Copy link
Contributor Author

bhollis commented May 3, 2019

Anything I can do to improve this PR? 2.0.1 is broken compared to 2.0.0 without this patch.

Copy link
Member

@markdalgleish markdalgleish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 Just pushed some minor updates before we merge.

(By the way, sorry—our Slack integration for this repo was misconfigured so we didn't see your comments)

@markdalgleish markdalgleish merged commit 6a97315 into seek-oss:master May 7, 2019
@plesiecki
Copy link

Sadly this change breaks compatibility with tivac/modular-css 😿

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants